-
-
Notifications
You must be signed in to change notification settings - Fork 307
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cmake build2 #348
Cmake build2 #348
Conversation
Hi, my hopes are high here as I got quite far even with the pre-PR versin, but with this version, I can't get beyond PostgreSQL. If I compile without it (cmake -DWITH_POSTGRES=NO ..), I get
If I compile with PostgreSQL (default), it ends with:
I'm on Ubuntu 18.04 (in Docker) with |
@wenzeslaus, Thanks for testing. You can try by running cmake: PS: if it works, I will push a fix that will not require anyone to provide -DPostgreSQL_TYPE_INCLUDE_DIR |
Yes, that's it. Thanks. I already figured that out. It seems I forgot to send a comment with a repo where you can see the test on Ubuntu VM: https://github.com/GRASS-GIS/grass-gis-experimental-ci/actions (branch cmake). (I'll follow up on the current issue there.) As for figuring it out, I eventually guessed from the error message that
You mean trying Besides that, my other issue was that with |
more like search using /postgresql/ among other things. cmake support it using PATH_SUFFIXES.
okay, I had to push a fix for it but cant do immediately, can you please test if -DWITH_POSGRES=TRUE, everything works?
Have to tried ccmake (curses ui) or cmake-gui. |
@wenzeslaus , I see an error from ctypegencore on your ci build (2 days ago) This issue has been fixed in other branch and is pushed here: You can simply build "ctypesgencore_py3" branch instead of this to test cmake. It contains this PR + more and does not have the error. Multiple PR are simply a way to split changes that are not related. |
I'm not sure what you want me to test, but the following works:
While leaving out I have used ccmake in the past, but it does not really seem to work well with scripting workflow for me. As for the build at GRASS-GIS/grass-gis-experimental-ci, the idea is to eventually have at least some part of it here, but at this point trying CMake build this way may create too much noise. I can just re-run the last test if needed from the website in this case. Generally, it updates with a change, so e.g. specifying the latest commit there force a particular version to be used, but that would be cumbersome. At this point, I'm expecting there there will be some changes needed anyway which will trigger the build (or on manually triggering the build). For right now, before you rebase this, I switched there to your ctypesgencore_py3, so you can see the result there. The next issue I got before was incomplete https://github.com/GRASS-GIS/grass-gis-experimental-ci/runs/470565935?check_suite_focus=true |
Thank a lot @wenzeslaus. I really appreciate your efforts in testing this PR. So far there are two issues postgresql and make install. I will push a patch to it soon. Once all cmake and msvc stuff are merged, I plan to add testing using ctest. Currently there is a prototype in r.info https://github.com/rkanavath/grass/blob/cmake_build2/raster/CMakeLists.txt#L136 |
Awesome! I'm really glad we are moving forward with this. Thanks for working on it.
Having better testing in place before merging msvc might be helpful, but for that, activating testing like in GRASS-GIS/grass-gis-experimental-ci might be enough.
Sounds good, but before you do that, please let's discuss what would be the advantage of this approach. I used CTest just once to run some Python tests of C++ code, so I don't know if that would bring any advantage to us. Perhaps more direct testing of C code in modules? Are you familiar with gunittest-based execution of all tests (which is customization on top of Python unittest)? With that said, I think using a standard test execution system such as CTest or pytest, perhaps alongside the more specialized gunittest execution, would be helpful. (I'm the original author of gunittest.) |
Idea is not to replace current testing with ctest. It is to run the same using ctest. Think of it as a wrapper/driver than can run test. ctest allow to have a dashboard (cdash) for these kinds of stuff. If one prefer to not use ctest and run gunittest that will be ofcourse doable. After all ctest is simply running them anyway. Also I prefer to add it after msvc support which has many pending PRs.. Otherwise it will simply spam up the list of pending PR for grass developers and me. |
@rkanavath, I have updated the code in this branch with code from master (e.g. Python 3 fixes) and with CI and submitted that as a PR against your repo, so you can decide how to incorporate that. Alternatively, I think I can commit to the branch directly or I can always create a new PR. What are your thoughts on this? I'm not really sure what you meant to accomplish with CTest, but I would leave that for later. I would just limit the goal for this PR to running something like:
which is actually what For this PR to be merged, I think we need: A rebase to get the latest code from master into this PR: Simple Even if it won't help with ctypesgencore, it will at least bring more CI tests to this PR which is another thing which needs to be added: CI build using CMake. I already put everything in place for this in the branch and it compiles, but the installation is lacking the main executable (grass.py). This is already a lot to review, so let's try to remove all what is not needed or put that to different PRs which will precede this one. The following four commits from your branch seem to contain things which are perhaps related, but not directly related to CMake. These should be already reduced on my updated branch. 628e04b Avoid use of bare-exception if you want to preserve the commits above in a different branch (and PR), you can just add the commits from this branch to a new one (while being on the new one) using Everybody, this is a lot of reviewing and moving code around in Git, so if you have different suggestions on how to proceed here and with the other related PRs, please share your ideas. |
This PR is an extract from OSGeo#348: - gui/wxpython/tools/build_modules_xml.py edits: https://github.com/OSGeo/grass/pull/348/files#diff-11ca77721b6d009b6537c54be4172efed843021797676258ad1141a6c63e6fd1 - lib/init/grass.py: https://github.com/OSGeo/grass/pull/348/files#diff-647be4ef599868c33f2f69f0d899f79f0aee8d75d1db74f523268c1ac94cddf7 Not sure if we want these changes but just to avoid to see them lost when closing OSGeo#348 in favour of OSGeo#2684.
# AUTHOR(S): Rashad Kanavath <rashad km gmail> | ||
# PURPOSE: CMake macro to build a grass executable (program) under sub directory | ||
# which is passed as argument to macro | ||
# COPYRIGHT: (C) 2000 by the GRASS Development Team |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Closing this PR in favour of Cmake PR #2684 (where this work has been carried over). Thanks to @rkanavath! |
* build_modules_xml.py, grass.py: catch errors This PR is an extract from #348: - gui/wxpython/tools/build_modules_xml.py edits: https://github.com/OSGeo/grass/pull/348/files#diff-11ca77721b6d009b6537c54be4172efed843021797676258ad1141a6c63e6fd1 - lib/init/grass.py: https://github.com/OSGeo/grass/pull/348/files#diff-647be4ef599868c33f2f69f0d899f79f0aee8d75d1db74f523268c1ac94cddf7
* build_modules_xml.py, grass.py: catch errors This PR is an extract from OSGeo#348: - gui/wxpython/tools/build_modules_xml.py edits: https://github.com/OSGeo/grass/pull/348/files#diff-11ca77721b6d009b6537c54be4172efed843021797676258ad1141a6c63e6fd1 - lib/init/grass.py: https://github.com/OSGeo/grass/pull/348/files#diff-647be4ef599868c33f2f69f0d899f79f0aee8d75d1db74f523268c1ac94cddf7
* build_modules_xml.py, grass.py: catch errors This PR is an extract from OSGeo#348: - gui/wxpython/tools/build_modules_xml.py edits: https://github.com/OSGeo/grass/pull/348/files#diff-11ca77721b6d009b6537c54be4172efed843021797676258ad1141a6c63e6fd1 - lib/init/grass.py: https://github.com/OSGeo/grass/pull/348/files#diff-647be4ef599868c33f2f69f0d899f79f0aee8d75d1db74f523268c1ac94cddf7
splitted commits of #289